DATAES-771 - Add after-save entity callbacks support. #414
Conversation
| protected <T> T maybeCallbackAfterSave(T entity) { | ||
|
|
||
| if (entityCallbacks != null) { | ||
| return entityCallbacks.callback(AfterSaveCallback.class, entity); |
There was a problem hiding this comment.
We should make sure that the value returned from the callback is not null. Although the API has neither the parameter or return value marked as nullable, we might get a null value by some bad implemention of an after-save callback. So I think we should assert this here.
There was a problem hiding this comment.
Looking at the code of SimpleEntityCallbackInvoker, it looke like it makes sure that the value returned by a callback is non-null:
throw new IllegalArgumentException(
String.format("Callback invocation on %s returned null value for %s", callback.getClass(), entity));
Also, javadoc for EntityCallbackInvoker#invokeCallback() says that the returned value is 'never null'.
So even a callback that returns null will just cause an exception somewhere in EntityCallbacks implementation, and here we will not get a null value.
There was a problem hiding this comment.
ah, I didn't dig this deep into the code for this PR, sorry (wish this were Kotlin stuff!), so that's ok
|
|
||
| // suppressing because it's either entity itself or something of a correct type returned by an entity callback | ||
| @SuppressWarnings("unchecked") | ||
| T castResult = (T) query.getObject(); |
There was a problem hiding this comment.
This must not be null, see my comment for the maybeCallbackAfterSave(T entity) method.
There was a problem hiding this comment.
query.getObject() can be null either if entity is null (which is impossible, there is an assertion in the beginning of the method), or if entityCallbacks.callback() returns null (which is also impossible).
So the castResult cannot be null.
Would you still prefer to assert that it is not null, just in case?
| @@ -151,7 +158,10 @@ public void setEntityCallbacks(EntityCallbacks entityCallbacks) { | |||
| }); | |||
| } | |||
|
|
|||
| return entities; | |||
| return indexQueries.stream() | |||
| .map(IndexQuery::getObject) | |||
There was a problem hiding this comment.
This must not be null, see my comment for the maybeCallbackAfterSave(T entity) method.
There was a problem hiding this comment.
Currently, this can only be null if some entity in the original Iterable is null.
Do you still suggest adding a non-null assertion?
| */ | ||
| @ExtendWith(MockitoExtension.class) | ||
| @MockitoSettings(strictness = Strictness.LENIENT) | ||
| public class ElasticsearchTransportTemplateUnitTests { |
There was a problem hiding this comment.
This should be ElasticsearchTransportTemplateCallbackTests, see my comment for the ElasticsearchRestTemplateUnitTests class.
There was a problem hiding this comment.
Renamed the test suite to ElasticsearchTransportTemplateCallbackTests, thanks for the suggestion
| */ | ||
| @ExtendWith(MockitoExtension.class) | ||
| @MockitoSettings(strictness = Strictness.LENIENT) | ||
| public class ElasticsearchRestTemplateUnitTests { |
There was a problem hiding this comment.
I think this should be better named ElasticsearchRestTemplateCallbackTests, otherwise it will be confusing with the already existing ElasticsearchTransportTemplateTests, and this class is especially for the callbacks. For the reactive part you have named it according to this pattern as well.
There was a problem hiding this comment.
Renamed the suite to ElasticsearchRestTemplateCallbackTests, thanks for the suggestion
This change enables to get callbacks when an entity gets saved to Elasticsearch.
|
Thank you for your review, @sothawo ! I've renamed the test suites. Concerning the null-checks, please see my comments. |
|
thanks a lot, this is merged. |
This change enables to get callbacks when an entity gets saved to Elasticsearch.
Comparing to AfterSaveCallback in spring-data-mongodb (that was used for inspiration), AfterSaveCallback added in this PR has a simpler method signature: it does not have neither
document(because Elasticsearch does not return an indexed document in a response from index operation) norcollectionarguments. We can addindexinstead ofcollectionthough. I need your input on this matter.Also, I'm going to add support for after-convert callbacks in the next PR if everything goes well with this one.